Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POtel implementation base branch #3152

Draft
wants to merge 143 commits into
base: master
Choose a base branch
from
Draft

POtel implementation base branch #3152

wants to merge 143 commits into from

Conversation

sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Jun 10, 2024

Contains:

Simple test

import sentry_sdk
from time import sleep

sentry_sdk.init(
    debug=True,
    traces_sample_rate=1.0,
    _experiments={"otel_powered_performance": True},
)

with sentry_sdk.start_span(description="sentry request"):
    sleep(0.1)
    with sentry_sdk.start_span(description="sentry db"):
        sleep(0.5)
        with sentry_sdk.start_span(description="sentry redis"):
            sleep(0.2)
    with sentry_sdk.start_span(description="sentry http"):
        sleep(1)

References

Misc

In OTel, this:

with tracer.start_as_current_span("parent") as parent:
    with tracer.start_span("child1"):
        pass
    with tracer.start_span("child2"):
        pass

is equivalent to

from opentelemetry import trace, context

parent = tracer.start_span("parent")

# Creates a Context object with parent set as current span
ctx = trace.set_span_in_context(parent)

# Set as the implicit current context
token = context.attach(ctx)

# Child will automatically be a child of parent
child1 = tracer.start_span("child1")
child1.end()

# Child will automatically be a child of parent
child2 = tracer.start_span("child2")
child2.end()

# Don't forget to detach or parent will remain the parent above this call stack
context.detach(token)
parent.end()

Copy link

codecov bot commented Jun 26, 2024

❌ 1710 Tests Failed:

Tests completed Failed Passed Skipped
20003 1710 18293 4413
View the top 1 failed tests by shortest run time
tests.test_utils test_datetime_from_isoformat[2021-01-01T00:00:00.000000-00:00-expected_output4]
Stack Traces | 0.001s run time
tests/test_utils.py:109: in test_datetime_from_isoformat
    assert datetime_from_isoformat(input_str) == expected_output, input_str
E   NameError: name 'datetime_from_isoformat' is not defined
View the full list of 2 ❄️ flaky tests
tests.test_utils test_datetime_from_isoformat[2021-01-01T00:00:00.000000-0000-expected_output6]

Flake rate in main: 58.06% (Passed 39 times, Failed 54 times)

Stack Traces | 0.001s run time
tests/test_utils.py:109: in test_datetime_from_isoformat
    assert datetime_from_isoformat(input_str) == expected_output, input_str
E   NameError: name 'datetime_from_isoformat' is not defined
tests.test_scope test_with_isolation_scope_data

Flake rate in main: 99.17% (Passed 2 times, Failed 239 times)

Stack Traces | 0.001s run time
tests/test_scope.py:387: in test_with_isolation_scope_data
    assert scope._tags == {"before_isolation_scope": 1}
E   AssertionError: assert {} == {'before_isolation_scope': 1}
E     
E     Right contains 1 more item:
E     {'before_isolation_scope': 1}
E     
E     Full diff:
E     + {}
E     - {
E     -     'before_isolation_scope': 1,
E     - }

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

@sl0thentr0py sl0thentr0py changed the title Skeletons for new POTEL components New POTEL base branch Jul 9, 2024
@sl0thentr0py sl0thentr0py changed the title New POTEL base branch potel implementation base branch Jul 9, 2024
sl0thentr0py and others added 4 commits July 22, 2024 16:50
Add simple scope management whenever a context is attached

* create a new otel context `_SCOPES_KEY` that will hold a tuple of
  `(curent_scope, isolation_scope)`
* the `current_scope` will always be forked (like on every span creation/context update in practice)
  * note that this is on `attach`, so not on all copy-on-write context
    object creation but only on apis such as
    [`trace.use_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L547)
    or [`tracer.start_as_current_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L329)
  * basically every otel `context` fork corresponds to our `current_scope` fork
* the `isolation_scope` currently will not be forked
  * these will later be updated, for instance when we update our top
    level scope apis that fork isolation scope, that will also have a
    corresponding change in this `attach` function
* only acts on `on_end` instead of both `on_start/on_end` as before
* store children spans in a dict mapping `span_id -> children`
* new dict only stores otel span objects and no sentry transaction/span objects so we save a bit of useless memory allocation
* I'm not using our current `Transaction/Span` classes at all to build the event because when we add our APIs later, we'll need to rip these out and we also avoid having to deal with the `instrumenter` problem
* if we get a root span (without parent), we recursively walk the dict and find the children and package up the transaction event and send it 
  * I didn't do it like JS because I think this way is better
  *  they [group an array of `finished_spans`](https://github.com/getsentry/sentry-javascript/blob/7e298036a21a5658f3eb9ba184165178c48d7ef8/packages/opentelemetry/src/spanExporter.ts#L132) every time a root span ends and I think this uses more cpu than what I did
  * and the dict like I used it doesn't take more space than the array either
* if we get a span with a parent we just update the dict to find the span later
* moved the common `is_sentry_span` logic to utils
szokeasaurusrex and others added 4 commits July 23, 2024 12:54
Remove `instrumenter` parameter from all functions that accept it
(details below), and modify tests to not pass the `instrumenter`
parameter to any functions that used to take it.

Also, delete `tests/tracing/test_noop_span.py`, which tests
functionality removed in this commit.

BREAKING CHANGE:

  - Remove `sentry_sdk.init`'s `instrumenter` kwarg.
  - Delete `sentry_sdk.contsts.INSTRUMENTER` class.
  - Remove `sentry_sdk.hub.Hub.start_span`'s `instrumenter` parameter.
  - Remove `sentry_sdk.hub.Hub.start_transaction`'s `instrumenter`
    parameter.
  - Remove `sentry_sdk.scope.Scope.start_transaction`'s `instrumenter`
    parameter.
  - Remove `sentry_sdk.scope.Scope.start_span`'s `instrumenter`
    parameter.
  - Remove `sentry_sdk.tracing.Span.start_child`'s `instrumenter`
    parameter.
  - Remove `sentry_sdk.tracing.NoOpSpan.start_child`'s
    `instrumenter` parameter.

Closes: #3321
Improved extraction of op, description, status and http_status for a Sentry span from an OpenTelemenetry span.

Fixes #3236

---------

Co-authored-by: Neel Shah <[email protected]>
Co-authored-by: Ivana Kellyerova <[email protected]>
@antonpirker antonpirker changed the title potel implementation base branch POtel implementation base branch Aug 5, 2024
szokeasaurusrex and others added 11 commits August 7, 2024 14:42
Also, remove any tests for `sentry_sdk.configure_scope`.

Since Strawberry's deprecated [Sentry tracing extensions](https://strawberry.rocks/docs/extensions/sentry-tracing) import `sentry_sdk.configure_scope`, importing `strawberry.extensions.tracing.SentryTracingExtension` (or `SentryTracingExtensionSync`) will result in an unhandled exception. Therefore, these imports, and any functionality associated with them, have also been removed. This itself is not a breaking change, as it is necessitated by the removal of `sentry_sdk.configure_scope`.

BREAKING CHANGE: Remove `sentry_sdk.configure_scope`.

Closes: #3402
Also, remove any tests that test `sentry_sdk.push_scope`.

BREAKING CHANGE: Remove `sentry_sdk.push_scope`.

Closes #3403
This change is a prerequisite for #3404.

BREAKING CHANGE: Remove `sentry_sdk.transport.HttpTransport`'s `hub_cls` attribute.
* Removed hub based functions from sessions implementation
* Removed scope manager
* Removed hub from tracing
* Removed hub from apidocs
* Updated migration guide
* Updated migration guide
* Skeletons for new components

* Add simple scope management whenever a context is attached

* create a new otel context `_SCOPES_KEY` that will hold a tuple of
  `(curent_scope, isolation_scope)`
* the `current_scope` will always be forked (like on every span creation/context update in practice)
  * note that this is on `attach`, so not on all copy-on-write context
    object creation but only on apis such as
    [`trace.use_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L547)
    or [`tracer.start_as_current_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L329)
  * basically every otel `context` fork corresponds to our `current_scope` fork
* the `isolation_scope` currently will not be forked
  * these will later be updated, for instance when we update our top
    level scope apis that fork isolation scope, that will also have a
    corresponding change in this `attach` function

* Don't parse DSN twice

* wip

* Skeletons for new components

* Skeletons for new components

* Add simple scope management whenever a context is attached

* create a new otel context `_SCOPES_KEY` that will hold a tuple of
  `(curent_scope, isolation_scope)`
* the `current_scope` will always be forked (like on every span creation/context update in practice)
  * note that this is on `attach`, so not on all copy-on-write context
    object creation but only on apis such as
    [`trace.use_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L547)
    or [`tracer.start_as_current_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L329)
  * basically every otel `context` fork corresponds to our `current_scope` fork
* the `isolation_scope` currently will not be forked
  * these will later be updated, for instance when we update our top
    level scope apis that fork isolation scope, that will also have a
    corresponding change in this `attach` function

* mypy fixes

* working span processor

* lint

* Port over op/description/status extraction

* defaultdict

* naive impl

* wip

* fix args

* wip

* remove extra docs

* Add simple scope management whenever a context is attached (#3159)

Add simple scope management whenever a context is attached

* create a new otel context `_SCOPES_KEY` that will hold a tuple of
  `(curent_scope, isolation_scope)`
* the `current_scope` will always be forked (like on every span creation/context update in practice)
  * note that this is on `attach`, so not on all copy-on-write context
    object creation but only on apis such as
    [`trace.use_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L547)
    or [`tracer.start_as_current_span`](https://github.com/open-telemetry/opentelemetry-python/blob/ba22b165471bde2037620f2c850ab648a849fbc0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L329)
  * basically every otel `context` fork corresponds to our `current_scope` fork
* the `isolation_scope` currently will not be forked
  * these will later be updated, for instance when we update our top
    level scope apis that fork isolation scope, that will also have a
    corresponding change in this `attach` function

* Implement new POTel span processor (#3223)

* only acts on `on_end` instead of both `on_start/on_end` as before
* store children spans in a dict mapping `span_id -> children`
* new dict only stores otel span objects and no sentry transaction/span objects so we save a bit of useless memory allocation
* I'm not using our current `Transaction/Span` classes at all to build the event because when we add our APIs later, we'll need to rip these out and we also avoid having to deal with the `instrumenter` problem
* if we get a root span (without parent), we recursively walk the dict and find the children and package up the transaction event and send it 
  * I didn't do it like JS because I think this way is better
  *  they [group an array of `finished_spans`](https://github.com/getsentry/sentry-javascript/blob/7e298036a21a5658f3eb9ba184165178c48d7ef8/packages/opentelemetry/src/spanExporter.ts#L132) every time a root span ends and I think this uses more cpu than what I did
  * and the dict like I used it doesn't take more space than the array either
* if we get a span with a parent we just update the dict to find the span later
* moved the common `is_sentry_span` logic to utils

* Basic test cases for potel (#3286)

* Proxy POTelSpan.set_data to underlying otel span attributes (#3297)

* ref(tracing): Simplify backwards-compat code (#3379)

With this change, we aim to simplify the backwards-compatibility code
for POTel tracing.

We do this as follows:
  - Remove `start_*` functions from `tracing`
  - Remove unused parameters from `tracing.POTelSpan.__init__`.
  - Make all parameters to `tracing.POTelSpan.__init__` kwarg-only.
  - Allow `tracing.POTelSpan.__init__` to accept arbitrary kwargs,
    which are all ignored, for compatibility with old `Span` interface.
  - Completely remove `start_inactive_span`, since inactive spans can
    be created by setting `active=False` when constructing a
    `POTelSpan`.

* New Scope implementation based on OTel Context (#3389)

* New `PotelScope` inherits from scope and reads the scope from the otel context key `SENTRY_SCOPES_KEY`
* New `isolation_scope` and `new_scope` context managers just use the context manager forking and yield with the scopes living on the above context key
  * isolation scope forking is done with the `SENTRY_FORK_ISOLATION_SCOPE_KEY` boolean context key

* Fix circular imports (#3431)

* Random tweaks (#3437)

* Origin improvements (#3432)

* Tweak OTel timestamp utils (#3436)

* Create spans on scope (#3442)

* Fill out more property/method stubs (#3441)

* Cleanup origin handling and defaults (#3445)

* add note to migration guide

* Attribute namespace for tags, measurements (#3448)

---------

Co-authored-by: Neel Shah <[email protected]>
Co-authored-by: Neel Shah <[email protected]>
Co-authored-by: Daniel Szoke <[email protected]>
antonpirker and others added 30 commits November 5, 2024 13:53
* Don't default to OK span status for UNSET, this breaks older behavior
* Set transaction name manually on scope after isolation (we should
  generally do this everywhere)
…3748)

If this option is on, we will only create a new underlying otel span if
there's an active valid parent, otherwise we will just return an invalid
`NonRecordingSpan` (`INVALID_SPAN`).

All internal integration child `start_span` calls have been modified so that
now we will only create spans if there is an active root span
(transaction) active.
mainly some middleware span reordering
* Fix transaction name setting and forked some tests to make them work in potel

* Transactions in transactins is undefined behavior, so remove this.
- Add support for the sampled flag for start_span and respect it when making sampling decisions.
- Rework sampling_context in traces_sampler to work with span attributes instead. Make sure we still have the same data accessible as now.

We could go one step further and change the format of sampling_context to just be the actual span attributes without any postprocessing into the current format. I kept the format in line with what we have now to make it easier to update.

See #3746
Closes #3739

This is a breaking change since we're removing custom_sampling_context. It'll break multiple integrations until we fix them (see #3746).
Remove explicit trace_id passing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants